Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Project page now shows "Ask to join project" button for org members #1390

Merged
merged 4 commits into from
Jan 24, 2025

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Jan 17, 2025

Fixes #1153.

Org members who aren't yet project members can ask to join a project by clicking the button, which will send an email to project managers.

image

Pops up a persistent notification that won't time out:

image

Org members who aren't yet project members can ask to join a project by
clicking the button, which will send an email to project managers.
@rmunn rmunn requested a review from hahn-kev January 17, 2025 19:59
Copy link

github-actions bot commented Jan 17, 2025

UI unit Tests

12 tests  ±0   12 ✅ ±0   0s ⏱️ ±0s
 4 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit 49d98c4. ± Comparison against base commit 30c8d5a.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jan 17, 2025

C# Unit Tests

104 tests  ±0   104 ✅ ±0   5s ⏱️ ±0s
 16 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 49d98c4. ± Comparison against base commit 30c8d5a.

♻️ This comment has been updated with latest results.

@rmunn
Copy link
Contributor Author

rmunn commented Jan 17, 2025

One issue that came up when I tested it, but shouldn't arise in production: the elawa project, at the time I tested this, had no members and so there was nobody to send the email to, yet the notification falsely told the user that the project managers had been emailed. However, in production we have a rule that projects without managers aren't allowed, so I think it's safe to ignore that situation.

Copy link
Collaborator

if it's an exceptional case then lets throw an exception for that, the UX isn't great but it's better than a false positive I think, and then we don't have to properly handle that case which should never happen.

Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, just a minor change of throwing an exception from the backend if there's no managers to email

@rmunn
Copy link
Contributor Author

rmunn commented Jan 21, 2025

looks good, just a minor change of throwing an exception from the backend if there's no managers to email

Done in commit 3401e2e, but I'm getting a Typescript error. Although I declared that the AskToJoinProject mutation can throw the new ProjectHasNoManagers exception, that's somehow not getting into the generated GQL types: when I run task api:generate-gql-schema, I see the ProjectHasNoManagers error in the schema but the AskToJoinProjectMutation type isn't updated with the new list of possible errors.

So this will fail to build (update: yes, it fails), but I'm pushing it anyway because it should be working and I need a bit of help figuring out why the GraphQL type generation is failing.

@rmunn rmunn self-assigned this Jan 22, 2025
@hahn-kev
Copy link
Collaborator

hahn-kev commented Jan 22, 2025

the issue was that joinResult.error did not have a __typename as it wasn't in the gql query, and byType only accepts the type name using some generic hoops to only accept valid values, in this case since it wasn't selected it was never. I wish we could instruct TS to show a custom error when trying to assign to a never type, but I don't think that's possible.

@rmunn
Copy link
Contributor Author

rmunn commented Jan 22, 2025

Okay, so it looks like the only thing remaining is to put an approving review on this and I can get it merged. (I could check the "bypass rules" checkbox, but there's no reason to skip steps in this case).

Copy link
Contributor

@myieye myieye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tweaked the button so we hide the mail icon when it's showing a loading indicator 🤓

@rmunn rmunn merged commit 9355ee2 into develop Jan 24, 2025
18 checks passed
@rmunn rmunn deleted the feat/org-members-can-ask-to-join-org-projects branch January 24, 2025 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow org members to join existing org projects
3 participants